Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix : deleted user when email share registers #3648

Merged
merged 1 commit into from
Aug 20, 2024
Merged

Conversation

hamza221
Copy link
Contributor

@hamza221 hamza221 commented Aug 14, 2024

Reproduction:

  1. Create a poll with a few options
  2. Share poll by mail
  3. open the share link received in the email
  4. Vote
  5. Register with the same email

Desired behaviour :
The new display name gets updated in the vote results
Actual behaviour :
The vote is displayed as deleted user

@dartcafe
Copy link
Collaborator

dartcafe commented Aug 15, 2024

I am struggeling to replay your Steps.
What I did:
1.-3.
After that I voted as the email owner. What do you mean with "register with the same email"?

@hamza221
Copy link
Contributor Author

After voting click Register
image
And use the same address you received the invitation on

@dartcafe
Copy link
Collaborator

dartcafe commented Aug 15, 2024

Ahhhhhh. OMG. OK. You mean using the same share address after using it for initial vote. There is definetely a bug, but the bug is, that reusing the link leads to the register page and not directly to the poll.

The share link is the direct ticket to enter the poll.

@dartcafe
Copy link
Collaborator

The register method is the wrong address, since it is used to create a new share after registering and should only use the userId created there. Strange. I wonder why this leads to the registration page.

@hamza221
Copy link
Contributor Author

hamza221 commented Aug 15, 2024

So if I understand correctly, the register button should only be shown for non logged-in users in public shares.
It shouldn't be shown for mail shares since an ID already exists (the email address )
Correct?

@hamza221
Copy link
Contributor Author

hamza221 commented Aug 15, 2024

maybe currentUser can be checked and the button should be rendered conditionally ?
image
Should I update the Pr ?

@dartcafe
Copy link
Collaborator

dartcafe commented Aug 15, 2024

I have to replay my memory. 📝

First use case

A public link is used and the user has to register. He then gets a new share token and will be redirected to the poll with the fresh created share.

Second use case

An external user is invited via mail or via contact (like mail but with the contact's name as display name) from the contact app.
In this case the user should be able to approve or change his email address and displayname first. I think this logic broke, when changing the workflow from first show the registration dialog to showing the poll without it and hiding the registration behind the register button.

I think I forgot to respect the second use case whan changing the workflow.

@dartcafe
Copy link
Collaborator

So changing the PR to quick fix that would be nice. Anyway, the userId is the email address, which is also not a good idea. It should be a gerneated id as well, since it is a personal public share.

But I am thinking about making next the main branch, once the vue3 migration is stable and think about a more robust change over there.

@dartcafe
Copy link
Collaborator

dartcafe commented Aug 15, 2024

maybe currentUser can be checked and the button should be rendered conditionally ?

Checking the share type is more precise. Or at least the permissions delivered in the poll structure. The permission votemeans the user is allowed to vote.

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>
@dartcafe
Copy link
Collaborator

It seems to do the job. But there is another problem. Changing the username or email address throws an error.

Seems, I mixed up something somewhere. 🙈

@dartcafe dartcafe added this to the 7.1.5 milestone Aug 19, 2024
@sorbaugh sorbaugh merged commit b1d4a03 into master Aug 20, 2024
21 checks passed
@sorbaugh
Copy link
Contributor

Hello @dartcafe, hope it's ok that I merged and leave the newly arisen problem to be tackled in a further PR!

@dartcafe
Copy link
Collaborator

OK for me atm. But I prefer the PR getting merged by the creator. Possible final action may be planned before merging.

At least the changelog is missing. But I will add it later.

@AndyScherzinger AndyScherzinger deleted the fix/delete-user branch August 20, 2024 15:10
@AndyScherzinger
Copy link
Member

/backport to next

Copy link

backportbot bot commented Aug 20, 2024

The backport to next failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout next
git pull origin next

# Create the new backport branch
git checkout -b backport/3648/next

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 2d09af31

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/3648/next

Error: No changes found in backport branch


Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

@dartcafe
Copy link
Collaborator

Backport to next is not possible, since it got a complete overwork.

@AndyScherzinger AndyScherzinger removed this from the 7.3.0 milestone Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants